-
Notifications
You must be signed in to change notification settings - Fork 123
Support Cluster Based Pinning for Routing Rules #796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
in PR description you say:
did you make sure to accommodate: |
RoeyoOgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, i might have missed but i don't see any new tests just fixing existing ones
| routingRules: | ||
| rulesEngineEnabled: False | ||
| # rulesConfigPath: "src/main/resources/rules/routing_rules.yml" | ||
| # rulesConfigPath: "gateway-ha/src/main/resources/rules/routing_rules.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have this change?
also why -ha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I'm testing with TrinoGatewayRunner, it sets the working directory to the repository root. For instance, to read the config.yaml file, we need to put gateway-ha directory in the path
| HaGatewayLauncher.main(new String[] {"gateway-ha/config.yaml"}); |
Wondering what's the reason we'd like to set it to rulesConfigPath: "src/main/resources/rules/routing_rules.yml"?
| { | ||
| Map<String, String> result = new HashMap<>(); | ||
| // Keep only the highest-priority rule result by limiting the map to a single entry. | ||
| LinkedHashMap<String, String> result = new LinkedHashMap<>(1) { @Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this this change needed? is this a current issue or changed by added behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I've added the Key Change Explanation section in the PR description to explain this change
Yes, If there's no cluster matching the rule exists, we would fall back to the trino-gateway/gateway-ha/src/main/java/io/trino/gateway/ha/router/BaseRoutingManager.java Line 120 in e28d4c2
I've also modified the PR description to make it clear. |
New tests are added in trino-gateway/gateway-ha/src/test/java/io/trino/gateway/ha/router/TestRoutingSelector.java Lines 562 to 615 in e28d4c2
|
Description
This PR adds Trino-cluster-based pinning as proposed in #789.
File-based routing rules can now specify actions like
"result.put(\"routingCluster\", \"trino-1\")"to pin directly to a specific backend cluster. The rules engine evaluates all rules and selects the highest-priority match, which can either be a routing group or a routing cluster.We also support external routing selector to return
routingClusteras aroutingDestination.Logic Changes
"result.put(\"routingCluster\", \"trino-1\")".FileBasedRoutingSelector.findRoutingDestination, we now ensure the result map contains exactly one entry so that only the highest-priority decision is kept.Key Change Explanation
All routing rules are sorted by priority. In
FileBasedRoutingSelector, we iterate through them from low to high priority, checking whether each rule matches. Each match overwrites the previous decision. By the end, we have the routing decision associated with the highest-priority rule.Previously, we only had one possible result key:
routingGroup, so following the behavior as described above, the resultHashMapwould return us only oneroutingGroupkey, with the value of the highest priority routing group.Now, we support both
routingGroupandroutingCluster, and using a regularHashMapcan return two results:routingGroupkey with the value of highest priority grouproutingClusterkey with the value of the highest priority cluster.At this point, we won't know which one actually has the higher priority.
To solve this issue, we switched the result map to a
LinkedHashMap, with the eviction mechanism ofremoveEldestEntryto keep the map size at one. This ensures that as we traverse rules, a higher-priority routing cluster can override a lower-priority routing group, and vice versa. Thus, after all the rules are traversed, we end up with exactly one result - either a routing group or a routing cluster - with the globally highest priority.Refactoring
RoutingGroupSelectorto the more generalRoutingSelector, with corresponding updates to all the classes implement it.RoutingGroupResponsetoRoutingResponseand added aroutingClusterfield.routingClusterfield inRoutingSelectorResponseroutingGroupinRoutingDestinationwith a more general variable nameroutingDecision, which can be either a routing group or a routing cluster. This change also applies to:query_historytable columnTesting
mvn clean installTestRoutingSelector